[RemoteStore]: Add support for repository with Server side encryption enabled and client side encryption as well based on a flag. #19630
Conversation
pranikum
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
pranikum
changed the title
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
pranikum
force-pushed
the
sse-blobstore-changes
branch
from
c6051f6 to
01b6ca2
Compare
Signed-off-by: Pranit Kumar <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19630 +/- ##
============================================
+ Coverage 73.11% 73.14% +0.02%
- Complexity 70661 70723 +62
============================================
Files 5724 5727 +3
Lines 323498 323651 +153
Branches 46852 46866 +14
============================================
+ Hits 236518 236721 +203
- Misses 67846 67853 +7
+ Partials 19134 19077 -57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
Signed-off-by: Pranit Kumar <[email protected]>
|
❕ Gradle check result for 3560a0a: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
| scopedSettings.addSettingsUpdateConsumer( | ||
| IndexMetadata.INDEX_REMOTE_STORE_SSE_ENABLED_SETTING, | ||
| this::setServerSideEncryptionEnabled | ||
| ); |
There was a problem hiding this comment.
This is not a dynamic setting , hence we don't need it .
Looks like Index's custom metadata is the right place for it .
| public static final String REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX = "remote_store.repository.%s.settings."; | ||
|
|
||
| public static final String REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT = "%s.repository.%s.type"; | ||
| public static final String REPOSITORY_SERVER_SIDE_ENCRYPTION_ATTRIBUTE_KEY_FORMAT = "%s.repository.%s.server_side_encryption_enabled"; |
There was a problem hiding this comment.
So to enable this we would need to have this in the repository metadata ? Can every repository have its own implementation for it rather ?
There was a problem hiding this comment.
Though the BlobStoreRepository implementation is generic. Will move any call to RemoteStore related code from BlobStore. However We will need RemoteStore specific implementation when adding index specific setting since that flow will be specific to remote store.
| } | ||
|
|
||
| public BlobStoreProvider getBlobStoreProvider() { | ||
| if (RemoteStoreNodeAttribute.isServerSideEncryptionEnabled(metadata.settings())) { |
There was a problem hiding this comment.
instead of RemoteStoreNodeAttribute , can BlobStoreRepository tell if it is SSE based on its own attributes . For ex in s3, if SSE-KMS related settings are present we can infer it is SSE already .
There was a problem hiding this comment.
Yes.. Will change this call to make it generic .
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| protected BlobStore createClientSideEncryptedBlobStore() { |
There was a problem hiding this comment.
repository doesn't know if it is client side encryption or not . That construct is outside repository .
Also we can use server side + client side both . Good to honor all the server side encryption related settings for all request .
| /** | ||
| * BlobStoreProvider for RemoteStoreProvider | ||
| */ | ||
| public class ServerSideEncryptionEnabledBlobStoreProvider extends BlobStoreProvider { |
There was a problem hiding this comment.
why do we need two providers , since the signature of blobStore itself contains serverSideEncryptionEnabled
BlobStore blobStore(boolean serverSideEncryptionEnabled)
| segmentRepo = RemoteStoreNodeAttribute.getSegmentRepoName(remoteNode.get().getAttributes()); | ||
| if (!isRestoreFromSnapshot | ||
| && RemoteStoreSettings.isServerSideEncryptionRepoEnabled(clusterState.nodes().getMinNodeVersion()) | ||
| && RemoteStoreNodeAttribute.isRemoteStoreServerSideEncryptionEnabled(remoteNode.get().getAttributes(), segmentRepo)) { |
There was a problem hiding this comment.
we can add a method in RemoteStoreNodeService about the same . RemoteStoreNodeService can use repository data and infer from it based on repository type.
Signed-off-by: Pranit Kumar <[email protected]>
|
❌ Gradle check result for cca7474: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
2 participants
Description
We will be adding support of having S3 repository to support both client side and server side encryption enabled blobs. Currently BlobStoreRepository only supports Single BlobStore created Lazily and once created the repository is tied to that BlobStore.
With this change we will be moving away from that design. We will be adding BlobStoreProvider which will be responsible for creating client side encrypted and server side encrypted BlobStore. Any Repository which needs this support can leverage by overriding appropriate builder methods.
Related Issues
[Resolves #[(https://github.com//issues/19235]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.